Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test function to validate Transition node ID immutability #18

Merged
merged 1 commit into from
Feb 20, 2021

Conversation

claudiosdc
Copy link
Contributor

I propose the addition of a new test function to contract::nodes module to help validate the solution for the Transition node ID mutability.

@dr-orlovsky dr-orlovsky added this to the v0.4.0 milestone Feb 19, 2021
@dr-orlovsky
Copy link
Member

Not sure why it has not run any CI. Am I right that currently this test fails?

@claudiosdc
Copy link
Contributor Author

Not sure why it has not run any CI. Am I right that currently this test fails?

Yes, you are correct. The changes that you have implemented are incomplete. I have been trying to warn you guys about it for a while. For one thing, the CommitEncode trait is currently implemented for Assignments using the UsingStrict strategy, which does not conceal the assignments.

If you look at my original PR (#6) you will see that the CommitEncode trait needs to be explicitly implemented for Assignments and OwnedState<STATE> (in addition to Void that has already been implemented). I have added several comments to #6 explaining the reasons for that.

@dr-orlovsky
Copy link
Member

The part on the RGB Core is WIP, so yes, it is incomplete. And I see no other way of meeting all spec requirements from https://github.com/LNP-BP/LNPBPs/discussions/88 other than this. The marker trait you proposed does not solve linearization problem for nested tree maps, and also we need to introduce proper merkle tree node tagging.

@dr-orlovsky dr-orlovsky reopened this Feb 19, 2021
@dr-orlovsky
Copy link
Member

Closed & reopened in attempt to make CI work and see that it fails on this test

@dr-orlovsky
Copy link
Member

Ouch, I see, the PR is against non-master, that's why CI is not running. Can you pls do another PR against master so we can see that the master does not pass this test (meaning that the test is correct)?

@claudiosdc
Copy link
Contributor Author

The part on the RGB Core is WIP, so yes, it is incomplete. And I see no other way of meeting all spec requirements from LNP-BP/LNPBPs#88 other than this. The marker trait you proposed does not solve linearization problem for nested tree maps, and also we need to introduce proper merkle tree node tagging.

@dr-orlovsky, I was not referring to the proposed change to the client_side_validation crate. Please forget about it for now, and pay attention to how the CommitEncode trait is currently implemented for the Assignments type.

https://github.com/rgb-org/rgb-core/blob/bf1bac19a27ec9a974d5d2412085d9081f9f3fcb/src/contract/assignments.rs#L406-L408

It should be using the UsingConceal strategy, not the UsingStrict strategy as it is now.

@dr-orlovsky
Copy link
Member

dr-orlovsky commented Feb 20, 2021

@claudiosdc Thanks for pointing that out, Assignments were set to UsingConceal few commits ago.

Anyway, #8 is finalized now, you are welcome to review (however it is rather large and complex, addressing many other issues than just node_id mutability).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants